feat(telemetry): expose OTel metrics via a Prometheus scrape endpoint#6034
Conversation
838dca0 to
e451304
Compare
rhdedgar
left a comment
There was a problem hiding this comment.
Nice, this covers all the criteria from RHAIENG-5156. +1
|
This pull request has merge conflicts that must be resolved before it can be merged. @cdoern please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
|
Looking good but I think we need changes around Auth and the Serviing port number If auth is enabled nothing will be able to scrape it unless is is configured with a token which could be problematic (at the very least would add complications) And if we disable Auth for /metrics the then we need to expose metrics on a separate port so that it is not exposed to the same users accessing the regular API |
OGX previously exported metrics only through OTLP push to an OTel Collector. This adds an optional Prometheus scrape endpoint so scrape-based monitoring systems can collect the existing metrics. When OGX_PROMETHEUS_ENABLED is set, setup_telemetry() attaches a PrometheusMetricReader to the MeterProvider alongside the existing OTLP reader, and the Inspect API serves all metrics at /v1/metrics in Prometheus exposition format. The endpoint opts out of authentication via PUBLIC_ROUTE_KEY, returns 404 when disabled, and is excluded from RequestMetricsMiddleware. The OTLP push path continues to work independently, so both export paths can run at once. Adds unit tests covering the Prometheus format and endpoint behavior, and a server-mode integration test that scrapes the live endpoint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Charlie Doern <cdoern@redhat.com>
Move the Prometheus scrape endpoint off the main API and onto a standalone HTTP server bound to its own port (OGX_PROMETHEUS_PORT, default 9464; OGX_PROMETHEUS_HOST, default 0.0.0.0). Serving metrics as a public route on the API port forced an awkward trade-off: behind authentication a scraper needs an API token, while making the route public exposes it to every API consumer. A separate port avoids both, letting collectors scrape without API authentication while keeping the endpoint unreachable from the regular API surface and isolatable at the network layer. setup_telemetry() now starts the scrape server via prometheus_client.start_http_server() when OGX_PROMETHEUS_ENABLED is truthy, alongside the existing OTLP push path. The in-API /v1/metrics route is removed from the Inspect router and no longer needs an auth bypass or exclusion from RequestMetricsMiddleware. Tests and documentation are updated accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Charlie Doern <cdoern@redhat.com>
e451304 to
1c49978
Compare
rhdedgar
left a comment
There was a problem hiding this comment.
I think the new changes look good. The two integration test job issues don't appear to be related.
| # Default to all interfaces so a scraper on another host/pod can reach it; | ||
| # operators can pin the bind address via OGX_PROMETHEUS_HOST. | ||
| port = _prometheus_port() | ||
| host = os.environ.get("OGX_PROMETHEUS_HOST", "0.0.0.0").strip() or "0.0.0.0" # noqa: S104 |
There was a problem hiding this comment.
Default bind address should be 127.0.0.1 - As its a Safer upstream default.
Users and Downstream deployments can override explicitly with OGX_PROMETHEUS_HOST=0.0.0.0 if required, this will ensure metrics are only available locally unless required
Also OGX_PROMETHEUS_HOST and OGX_PROMETHEUS_PORT seems misleading, Prometheus isn't being started, maybe OGX_METRICS_HOST and OGX_METRICS_PORT ?
metrics wont be listening on IPv6 by default, maybe this is ok, just mentioning
| value=raw, | ||
| default=_DEFAULT_PROMETHEUS_PORT, | ||
| ) | ||
| return _DEFAULT_PROMETHEUS_PORT |
There was a problem hiding this comment.
I don't think this should fallback to _DEFAULT_PROMETHEUS_PORT, if something has been misconfigured the server shouldn't start
|
|
||
| def _is_prometheus_enabled() -> bool: | ||
| """Return True if the Prometheus scrape endpoint is enabled via environment.""" | ||
| return os.environ.get("OGX_PROMETHEUS_ENABLED", "").strip().lower() in ("1", "true", "yes", "on") |
There was a problem hiding this comment.
Same naming problem here as with port, we're not enabling prometheus, we're enabling metrics, maybe OGX_METRICS_ENDPOINT_ENABLED ?
| metric_readers.append(PrometheusMetricReader()) | ||
| start_http_server(port=port, addr=host) | ||
| logger.info( | ||
| "OpenTelemetry Prometheus metrics reader configured, scrape endpoint exposed", |
There was a problem hiding this comment.
Could we disable this code during list-deps ?
dhiggins@dhiggins-thinkpadp1gen7:~/workarea/ogx$ OGX_PROMETHEUS_ENABLED=1 uv run ogx stack list-deps --format uv config-simple.yaml
INFO 2026-06-26T09:14:35.068961Z ogx.telemetry:106 OpenTelemetry Prometheus metrics reader configured, scrape
endpoint exposed category=telemetry host=0.0.0.0 port=9464
uv pip install pymongo aiosqlite scikit-learn 'mcp>=1.23.0,<2.0' matplotlib pillow 'pypdf>=6.13.0' 'fonttools>=4.60.2' redis faiss-cpu asyncpg anthropic markitdown[all] chardet pandas sqlalchemy[asyncio] aiosqlite fastapi fire httpx uvicorn opentelemetry-sdk
opentelemetry-exporter-otlp-proto-http opentelemetry-exporter-otlp-proto-grpc
|
This pull request has merge conflicts that must be resolved before it can be merged. @cdoern please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
|
Now that issue 6197 has been fixed, those two failing |
Rework the metrics scrape endpoint per review: - Rename the environment variables to reflect that this is a generic metrics endpoint, not a Prometheus deployment: OGX_PROMETHEUS_ENABLED becomes OGX_METRICS_ENDPOINT_ENABLED, OGX_PROMETHEUS_PORT becomes OGX_METRICS_PORT, and OGX_PROMETHEUS_HOST becomes OGX_METRICS_HOST. - Default the bind address to loopback (127.0.0.1) instead of all interfaces, so metrics are not exposed off-host unless an operator explicitly opts in by setting OGX_METRICS_HOST. - Fail fast when OGX_METRICS_PORT is not a valid integer rather than silently falling back to the default, so a misconfiguration surfaces at startup. - Defer binding the scrape server's port to start_metrics_server(), invoked from the server run path, so commands that merely import the telemetry module (such as ogx stack list-deps) no longer open a network port. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Charlie Doern <cdoern@redhat.com>
|
Thanks @derekhiggins — addressed all four in
On the IPv6 note: with the loopback default it binds IPv4 |
| except ValueError as e: | ||
| raise ValueError(f"Failed to parse OGX_METRICS_PORT as an integer: {raw!r}") from e | ||
|
|
||
| def setup_telemetry() -> None: |
There was a problem hiding this comment.
You moved where the http server for metrics is started but We're still setting up telemetry during list-deps, I think we can move all of this to server run path so it doesn't run during list-deps etc...
dhiggins@dhiggins-thinkpadp1gen7:~/workarea/ogx$ OGX_METRICS_ENDPOINT_ENABLED=1 uv run ogx stack list-deps --format uv config-simple.yaml
INFO 2026-07-01T09:55:55.787332Z ogx.telemetry:103 OpenTelemetry metrics scrape reader configured
category=telemetry
uv pip install scikit-learn sqlalchemy[asyncio] pillow 'mcp>=1.23.0,<2.0' faiss-cpu markitdown[all] 'pypdf>=6.13.0' anthropic aiosqlite matplotlib 'fonttools>=4.60.2' asyncpg redis chardet pandas pymongo aiosqlite fastapi fire httpx uvicorn opentelemetry-sdk opentelemetry-exporter-otlp-proto-http opentelemetry-exporter-otlp-proto-grpc
Previously setup_telemetry() ran at module import time, so any command that imported an ogx.telemetry submodule configured telemetry as a side effect. In particular `ogx stack list-deps` set up the MeterProvider and logged that the metrics scrape reader was configured, even though it never serves requests. Move configuration onto the path where a stack is actually brought up: - Call setup_telemetry() from Stack.initialize(), the shared entry point for both server and library-client modes, and drop the import-time call. Commands that only inspect configuration, such as `ogx stack list-deps`, never build a Stack and so no longer configure telemetry or open a network port. Placing it here rather than only in create_app() preserves OTLP export for library-client users, who never go through the server app factory. - Keep start_metrics_server() in create_app(), since exposing the scrape port is a server-mode concern. Because instruments in ogx.core.server.metrics are created at import, before Stack.initialize() runs, they start as OpenTelemetry proxy instruments and rebind to the real MeterProvider once it is installed, so metric collection is unaffected. setup_telemetry() now guards against reconfiguration by checking whether a real SDK MeterProvider is already installed (get_meter_provider() returns a proxy until set), which keeps a repeated Stack.initialize() in one process from registering a duplicate Prometheus collector. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Charlie Doern <cdoern@redhat.com>
|
|
||
| # Create and set global MeterProvider | ||
| provider = MeterProvider(resource=resource, metric_readers=[reader]) | ||
| provider = MeterProvider(resource=resource, metric_readers=metric_readers) |
There was a problem hiding this comment.
Hmm, looks like there could be a problem here when used with opentelemetry-instrument you see the following log on startup
WARNING 2026-07-01T16:31:27.632525Z Overriding of current MeterProvider is not allowed category=uncategorized
The opentelemetry-instrument bootstrap already handles OTLP export, so setup_telemetry()'s provider never takes effect on /metrics (we see some general process metrics but no ogx metrics)
There was a problem hiding this comment.
Fixed in cdc3edd6. setup_telemetry() now detects an already-installed MeterProvider (the opentelemetry-instrument case) and adds the Prometheus reader to it via add_metric_reader() instead of installing a competing provider. Plain ogx run creates one as before.
Verified live under opentelemetry-instrument: no override warning, and /metrics exposes ogx_requests_total. Added unit tests for both branches.
…metry-instrument
When ogx is launched with `opentelemetry-instrument` (the documented way to run
it with telemetry), the auto-instrumentation installs the global MeterProvider
and owns OTLP export. setup_telemetry() then tried to install its own provider,
which OpenTelemetry refuses ("Overriding of current MeterProvider is not
allowed"), so the provider carrying the Prometheus reader never took effect and
the scrape endpoint exposed no ogx metrics.
Detect the already-installed provider and add the Prometheus scrape reader to it
via MeterProvider.add_metric_reader() instead of installing a competing one. ogx
meters are already bound to that global provider, so they now reach the scrape
endpoint. When no provider exists (plain `ogx run`), a new one is created as
before.
Also move the single entry point onto the stack bring-up path so non-serving
commands stay unaffected: initialize_telemetry() (setup + scrape server start,
guarded to run once) is called from Stack.initialize(), which both server and
library modes reach but `ogx stack list-deps` does not. This replaces the
separate calls from create_app() and keeps src/ogx/core/stack.py under the file
size limit.
Verified against a live server: with `opentelemetry-instrument ogx run`, the
override warning is gone and /metrics exposes ogx_requests_total; plain `ogx run`
is unchanged. Adds unit tests for both provider branches and initialize_telemetry
idempotency.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Charlie Doern <cdoern@redhat.com>
… into rhaieng-5156-prometheus-metrics
What
Adds an optional metrics scrape endpoint that exposes the existing OTel metrics in Prometheus exposition format, alongside the current OTLP push path. This unblocks scrape-based monitoring systems that need a scrape endpoint rather than OTLP push.
Metrics are served from a standalone HTTP server on a dedicated port (default
9464), separate from the main API. Serving them as a route on the API port forced an awkward trade-off: behind authentication a scraper needs an API token, while making the route public exposes it to every API consumer. A separate port avoids both — collectors scrape without API authentication, and the endpoint stays unreachable from the regular API surface and isolatable at the network layer. By default it binds to loopback, so it is not exposed off-host unless an operator opts in.Resolves RHAIENG-5156.
How
opentelemetry-exporter-prometheus(pulls inprometheus-client).setup_telemetry()(src/ogx/telemetry/__init__.py): builds a list of metric readers — the existing OTLPPeriodicExportingMetricReaderwhenOTEL_EXPORTER_OTLP_ENDPOINTis set, plus aPrometheusMetricReaderwhenOGX_METRICS_ENDPOINT_ENABLEDis truthy. Both attach to a single globalMeterProvider, so the two export paths run independently. This runs at import time and only registers the reader; it does not open a port.start_metrics_server()(src/ogx/telemetry/__init__.py): binds the standalone scrape server viaprometheus_client.start_http_server(). It is called from the server run path (create_app), not at import, so commands that merely import the telemetry module (e.g.ogx stack list-deps) do not open a network port. It serves the default Prometheus registry thatPrometheusMetricReaderwrites to. Because it is not part of the FastAPI app, it bypasses auth by construction and never touchesRequestMetricsMiddleware. A non-integerOGX_METRICS_PORTfails startup rather than silently falling back to the default.Configuration
OGX_METRICS_ENDPOINT_ENABLED1/true/yes/on).OGX_METRICS_PORT9464OGX_METRICS_HOST127.0.0.10.0.0.0(or a specific address) to expose off-host.Acceptance criteria
OGX_METRICS_ENDPOINT_ENABLEDTest plan
Unit tests (
tests/unit/telemetry/test_prometheus_metrics.py): env-flag parsing, port parsing (default / override / invalid raises), and Prometheus-format exposition with labels and values.Verified the bind is deferred: importing
ogx.telemetrywithOGX_METRICS_ENDPOINT_ENABLED=1does not open the port (thelist-depspath), while callingstart_metrics_server()does and scrapes return200.Integration tests (
tests/integration/inspect/test_metrics_endpoint.py, server mode): scrape the live scrape server on its dedicated port over raw HTTP, assert Prometheus format andogx_requests_total, no-auth access, and that the API port returns404for/v1/metrics(metrics live off the API).scripts/integration-tests.shsetsOGX_METRICS_ENDPOINT_ENABLEDfor native server-mode runs; the tests skip otherwise.🤖 Generated with Claude Code